feat(AVM): Fail on incomplete toRadix decompositions by sirasistant · Pull Request #17233 · AztecProtocol/aztec-packages

sirasistant · 2025-09-23T14:28:53Z

Resolves #17144

fcarreiro · 2025-09-24T09:24:09Z

barretenberg/cpp/src/barretenberg/vm2/simulation/standalone/pure_to_radix.bench.cpp

-        auto value = fr::random_element();
        uint32_t num_limbs = static_cast<uint32_t>(state.range(0));
        uint32_t radix = static_cast<uint32_t>(state.range(1));
+        uint256_t value = 0;


could you add a comment about what you are doing here?

fcarreiro · 2025-09-24T09:24:56Z

barretenberg/cpp/src/barretenberg/vm2/simulation/standalone/pure_to_radix.cpp

    bool invalid_bitwise_radix = is_output_bits && (radix != 2);
    // Error handling - if num_limbs is zero, value needs to be zero
-    bool invalid_num_limbs = (num_limbs == 0) && value.is_zero();
+    bool invalid_num_limbs = (num_limbs == 0) && (!value.is_zero());


() not needed

fcarreiro · 2025-09-24T09:26:45Z

barretenberg/cpp/src/barretenberg/vm2/simulation/standalone/pure_to_radix.cpp

        throw ToRadixException("Invalid parameters for ToRadix");
    }

-    if (is_output_bits) {


I preferred using this form because the bits version is likely to be far faster than the non-bits version.

Also, it would be great to not reimplement here and be able to call the other ones?

I understand you reimplemented because you needed to know if it truncated... maybe consider returning std::pair<vector<>, /*truncated*/bool> from the lower level methods?

Making the low level one return a truncated boolean would help cleaning up the constrained one too

fcarreiro · 2025-09-24T09:47:17Z

barretenberg/cpp/src/barretenberg/vm2/simulation/standalone/pure_to_radix.cpp

    return limbs;
 }

 std::vector<bool> PureToRadix::to_le_bits(const FF& value, uint32_t num_limbs)


Just a note that I don't think this one is being exercised in bulk, depending on your confidence you might want to add a test file (until I sort out how to best share the tests)

[or add it to the bulk test via the opcode, if you make the opcode use this]

fcarreiro · 2025-09-24T10:55:30Z

barretenberg/cpp/src/barretenberg/vm2/simulation/standalone/pure_to_radix.bench.cpp

@@ -44,11 +44,14 @@ void VM_pure_to_radix_memory(State& state)
    for (auto _ : state) {


[Re: line +40]

Can you fix my typo? VM_ -> BM_

See this comment inline on Graphite.

jeanmon

Good work overall. Please consider the comments before merging. You might consider the suggested comparison (value and radix to the power num_limbs) to check if a truncation appeared in the simulation code.

barretenberg/cpp/pil/vm2/to_radix_mem.pil

jeanmon · 2025-09-24T11:11:23Z

barretenberg/cpp/pil/vm2/to_radix_mem.pil

+    pol commit sel_should_decompose;
+    sel_should_decompose * (1 - sel_should_decompose) = 0;
+
+    // On the start row, we define sel_should_decompose as !validation_error && !num_limbs_is_zero


Suggested change

// On the start row, we define sel_should_decompose as !validation_error && !num_limbs_is_zero

// On the start row, we define sel_should_decompose as !input_validation_error && !num_limbs_is_zero

jeanmon · 2025-09-24T11:12:44Z

barretenberg/cpp/pil/vm2/to_radix_mem.pil

        * (1 - sel_radix_gt_256_err) * (1 - sel_invalid_bitwise_radix)
        * (1 - sel_invalid_num_limbs_err);

+    pol commit output_limb_value;


A comment here about what is output_limb_value and value_found would be welcome.

Maybe a rename to limb_value would be less confusing IMHO.

jeanmon · 2025-09-24T11:15:50Z

barretenberg/cpp/pil/vm2/to_radix_mem.pil

+    pol commit sel_truncation_error;
+    sel_truncation_error * (1 - sel_truncation_error) = 0;
+    // A truncation error happens if in the start row, we look up the to_radix gadget and value_found is off.
+    // The to radix gadget is little endian, so the first row that we lookup is the last limb. If it's not found in the last limb,


Suggested change

// The to radix gadget is little endian, so the first row that we lookup is the last limb. If it's not found in the last limb,

// The to_radix gadget is little endian, so the first row that we lookup is the last limb. If it's not found in the last limb,

jeanmon · 2025-09-24T11:48:08Z

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp

+
+    // Negative test: disable decomposition after the start row:
+    trace.set(Column::to_radix_mem_sel_should_decompose, 2, 0);
+    EXPECT_THROW_WITH_MESSAGE((check_relation<to_radix_mem>(trace, to_radix_mem::SR_SEL_SHOULD_WRITE_MEM_CONTINUITY)),


I think you want to test another relation or you need to "reset the trace", otherwise it is clear that the relation will fail again.

Ah it's a typo. I meant to check the decompose selector continuity. will fix

jeanmon · 2025-09-24T12:00:57Z

barretenberg/cpp/src/barretenberg/vm2/constraining/relations/to_radix.test.cpp

+    check_interaction<ToRadixTraceBuilder, lookup_to_radix_mem_check_radix_lt_2_settings>(trace);
+}
+
+TEST(ToRadixMemoryConstrainingTest, TruncationError)


Can we craft a negative test like setting the truncation error while everything is fine and/or the othe way around the error is not toggled when it should?

jeanmon · 2025-09-24T12:05:53Z

barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/to_radix.cpp

+{
+    FF value = FF(0);
+    for (const auto& limb : limbs) {
+        value = value * radix + limb.as_ff();


Maybe an explicit cast for radix into FF would not be bad.

jeanmon · 2025-09-24T12:17:49Z

barretenberg/cpp/src/barretenberg/vm2/simulation/gadgets/to_radix.cpp

-            memory.set(dst_addr + i, be_output_limbs[i]);
-        }
+    // We need to reconstruct the value to check for truncation. The alternative would be to change the interface
+    // to return a truncated flag.


We can also raise the radix to the power of num_limbs and compare to the value.
In other words, if value < radix^num_limbs, we know for sure that it is not truncated.

We need to be careful with overflowing the field (or uint256_t) while raising radix to the power of num_limbs though.

jeanmon · 2025-09-24T12:28:41Z

yarn-project/simulator/src/public/avm/opcodes/conversion.test.ts

      },
    );

+    it('Should throw an error for a number that cant be decomposed in the given number of limbs', async () => {


Suggested change

it('Should throw an error for a number that cant be decomposed in the given number of limbs', async () => {

it('Should throw an error for a number that cannot be decomposed in the given number of limbs', async () => {

fcarreiro

LGTM with the small changes that are left

fcarreiro · 2025-09-24T13:18:53Z

barretenberg/cpp/src/barretenberg/vm2/simulation/standalone/pure_to_radix.cpp

-    */
-
-    std::vector<uint8_t> limbs = to_le_radix(value, num_limbs, 2);
+    const auto& [limbs, truncated] = to_le_radix(value, num_limbs, 2);


danger danger, you want const auto without & here ;)

this only doesn't blow up because of https://stackoverflow.com/questions/39718268/why-do-const-references-extend-the-lifetime-of-rvalues

fcarreiro · 2025-09-24T13:19:15Z

barretenberg/cpp/src/barretenberg/vm2/simulation/standalone/pure_to_radix.cpp

    bool invalid_bitwise_radix = is_output_bits && (radix != 2);
    // Error handling - if num_limbs is zero, value needs to be zero
-    bool invalid_num_limbs = (num_limbs == 0) && value.is_zero();
+    bool invalid_num_limbs = (num_limbs == 0) && (!value.is_zero());


jeanmon · 2025-09-24T13:37:25Z

barretenberg/cpp/src/barretenberg/vm2/simulation/standalone/pure_to_radix.cpp

+        if (truncated) {
+            throw ToRadixException("Truncation error");
+        }
+        std::reverse(limbs.begin(), limbs.end());


std::ranges::reverse() to be consistent with code below.

it doesn't work on vectors of bits

BEGIN_COMMIT_OVERRIDE feat(AVM): Fail on incomplete toRadix decompositions (#17233) END_COMMIT_OVERRIDE

Resolves #17144

sirasistant added 2 commits September 23, 2025 12:58

wip

58ec732

tests

815d5c3

sirasistant requested review from IlyasRidhuan, Maddiaa0, dbanks12, fcarreiro and jeanmon as code owners September 23, 2025 14:28

sirasistant changed the base branch from next to merge-train/avm September 23, 2025 14:29

sirasistant added 5 commits September 23, 2025 14:29

delete extraneous change

72da81c

Merge branch 'merge-train/avm' into arv/to_radix_decomposition

b54edec

fix pure to radix

0903af8

fix endianness

b8082c6

fix

52243c2

sirasistant removed request for IlyasRidhuan, Maddiaa0, dbanks12, fcarreiro and jeanmon September 24, 2025 08:42

sirasistant added 3 commits September 24, 2025 09:02

rename

547f842

Merge branch 'merge-train/avm' into arv/to_radix_decomposition

85ad5d8

regen after merge

5fb3753

sirasistant requested review from fcarreiro and jeanmon September 24, 2025 09:14

fcarreiro reviewed Sep 24, 2025

View reviewed changes

add comment

4eb52cc

fcarreiro reviewed Sep 24, 2025

View reviewed changes

jeanmon approved these changes Sep 24, 2025

View reviewed changes

address PR comments

6bcfef8

sirasistant requested a review from LeilaWang as a code owner September 24, 2025 13:15

fcarreiro reviewed Sep 24, 2025

View reviewed changes

sirasistant added 2 commits September 24, 2025 13:20

remove parenthesis

f570947

avoid taking a reference to a value that can be dropped

ec098f4

sirasistant removed the request for review from LeilaWang September 24, 2025 13:28

jeanmon reviewed Sep 24, 2025

View reviewed changes

sirasistant merged commit f54d94e into merge-train/avm Sep 24, 2025
6 checks passed

sirasistant deleted the arv/to_radix_decomposition branch September 24, 2025 15:11

AztecBot mentioned this pull request Sep 24, 2025

feat: merge-train/avm #17267

Merged

github-merge-queue bot pushed a commit that referenced this pull request Sep 24, 2025

feat: merge-train/avm (#17267)

244259c

BEGIN_COMMIT_OVERRIDE feat(AVM): Fail on incomplete toRadix decompositions (#17233) END_COMMIT_OVERRIDE

mralj pushed a commit that referenced this pull request Oct 13, 2025

feat(AVM): Fail on incomplete toRadix decompositions (#17233)

4ed4f57

Resolves #17144

ludamad pushed a commit that referenced this pull request Dec 16, 2025

feat(AVM): Fail on incomplete toRadix decompositions (#17233)

7166aca

Resolves #17144

		@@ -44,11 +44,14 @@ void VM_pure_to_radix_memory(State& state)
		for (auto _ : state) {

	// On the start row, we define sel_should_decompose as !validation_error && !num_limbs_is_zero
	// On the start row, we define sel_should_decompose as !input_validation_error && !num_limbs_is_zero

	// The to radix gadget is little endian, so the first row that we lookup is the last limb. If it's not found in the last limb,
	// The to_radix gadget is little endian, so the first row that we lookup is the last limb. If it's not found in the last limb,

	it('Should throw an error for a number that cant be decomposed in the given number of limbs', async () => {
	it('Should throw an error for a number that cannot be decomposed in the given number of limbs', async () => {

Conversation

sirasistant commented Sep 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeanmon left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sirasistant Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jeanmon Sep 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fcarreiro left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sirasistant commented Sep 23, 2025 •

edited

Loading

sirasistant Sep 24, 2025 •

edited

Loading

jeanmon Sep 24, 2025 •

edited

Loading